Reopen log file on SIGHUP #917
Conversation
|
Will this compile on windows/OSX/...? |
|
It does compile on OS X. I don't have a Windows build machine, but Microsoft documentation suggests that they support the necessary APIs. |
|
"Microsoft Windows Services for UNIX" != Windows. flockfile is not available on Windows (or at least not for MingW), and compiling it in next-test fails... |
|
I'll update the pull request to only activate One could also try using |
|
"Nobody" has a Windows development machine. Windows builds are produced with gitian on Ubuntu. |
|
I've updated the branch so that it's a noop on Windows |
| char pszFile[MAX_PATH+100]; | ||
| GetDataDir(pszFile); | ||
| strlcat(pszFile, "/debug.log", sizeof(pszFile)); | ||
| const char* pszFile = GetDebugLogName().c_str(); |
There was a problem hiding this comment.
Won't the std::string temporary from GetDebugLogName() free pszFile before it is used?
There was a problem hiding this comment.
I don't know. I've not encountered any problems running with this code. My C++ chops are weak, so I'll gladly do this differently if there's a better way.
There was a problem hiding this comment.
This is the equivalent of (in C):
char *s = malloc(8);
memcpy(s, "testing", 8);
free(s);
fopen(s, "a");
Try this:
std::string file = GetDebugLogName();
fileout = fopen(file.c_str(), "a");
|
ACK. Planning to (user-optionally) merge this in Gentoo for logrotate support. |
|
Looks like a useful feature. How does this interact with the log rotation / debug.log size-limiting inside bitcoin itself? I remember seeing some code for that. |
|
I don't know of any log rotation code inside bitcoin, but this code works well with the size limiting code. |
|
Ok, so that only runs at startup. That's good. ACK |
|
-1 from me. I think bitcoin should re-examine the bitcoin.conf file upon receiving a HUP. This, AFAIK is the more standard thing to happen on Unix, isn't it? |
|
The one does not exclude the other. And keep your -1/+1 out of here please. |
|
@laanwj You mean rotate the log file and re-parse bitcoin.conf? -0.99999 to that one ;) |
|
Yes. That's what most daemons do AFAIK and is sane. Both reopening the log |
|
I think it makes more sense to have a config option to specify how many logs files to keep and how often to rotate them. Having the log file rotated every time you want to re-read the config would be not very useful IMHO, as there may be 10000 lines added since the last config change or only 10 lines. |
|
I see no reason at all to include log rotation into bitcoin itself. There are excellent log rotation tools in common use. The only support they |
|
@laanwj I'm not aware of many people using Windows XP (for example) that use log rotation tools, and even if they did, are the tools able to rotate the logs at exactly midnight so that entries from either side of midnight don't end up in the wrong log file? |
|
I would recommend using SIGHUP for config file reloading and SIGUSR1 for logrotation. This is how most daemons do it and how it should be used with logrotate. That's whats needed for the Gentoo ebuild logrotate use flag and I am sure other distributions would benefit from it too. Rotation of log files should not be done by bitcoind. I am sure there are solutions for win32 users also. |
|
All three examples in the logrotate man page use SIGHUP. The default signal for newsyslog (used on OS X and other BSDs) is SIGHUP. As best I can tell, that's the closest thing to a standard that exists. |
|
On the other hand, many prominent FOSS uses SIGUSR1:
And my expectation about signals and daemons is that SIGHUP reloads the config file and SIGUSR1 re-opens the log file. Since both actions are not really related to each other, this seems to be a saner approach. And the stackoverflow answer shows that the community thinks so too. |
|
All those FOSS you just listed also use USR1 for reloading configs... But can we just pick one, any one, or maybe even both, and not spend pages discussing the pros/cons of each? |
|
IMHO configuration hot-reload isn't very useful... just my two satoshis... |
|
@davout I agree that at present config reload isn't very useful. I'm tempted to suggest that SIGHUP be reserved for that possibly functionality in future though, and going with SIGUSR1 for the logfile rotation, if that's the agreed standard. e.g. how does "tor" do things? I still think it's not unreasonable for bitcoin to offer to do it's own log rotation though - people can still choose to use a 3rd party program if they prefer. one or two config options would probably suffice, and it'd be easy to code. |
|
@rebroad it is unreasonable to reinvent the wheel. if you don't care about log rotation you'll be fine with the log size limit. if you do care you'll use logrotate. |
|
@rebroad: We want to reduce the complexity of the core, not increase it. Anything that can (sanely) be handled by external utilities, should be. If your platform does not have these utilities, you can port them or switch platform. Note that log rotation is only useful for servers and services, in which you'd like to keep the old data for auditing purposes. Windows XP users tend to be normal end-users which don't care about log files at all. Reloading configuration is an orthogonal option, open a new issue and discuss about it there. Let's keep this thread for testing and ACKing this code. |
Let's get this merged. |
|
Code review comments:
|
|
I assumed that the point of using file locks is you can block the logger from your log rotation tool, so you don't lose log records during the transition? |
1) Is freopen(3) guaranteed to always return orig_stream? My understanding of the documentation on Linux and OS X, leads me to say yes. I'm certain that boost has a thread locking primitive that may be statically initialized, and available immediately at startup I've not found such a primitive in the Boost libraries, but may not recognize it if I saw it. I'll gladly change the locking technique if someone can point me at a better primitive. |
|
|
Rebased and updated to use |
|
Changes look good, except for one: you made 'fileout' a global variable, when the existing, more narrowly-scoped 'static' declaration is preferred. Might consider making that static mutex more narrowly scoped like 'fileout', too. |
Acquire an exclusive, advisory lock before sending output to debug.log and release it when we're done. This should avoid output from multiple threads being interspersed in the log file. We can't use CRITICAL_SECTION machinery for this because the debug log is written during startup and shutdown when that machinery is not available. (Thanks to Gavin for pointing out the CRITICAL_SECTION problems based on his earlier work in this area)
The best log rotation method formerly available was to configure logrotate with the copytruncate option. As described in the logrotate documentation, "there is a very small time slice between copying the file and truncating it, so some logging data might be lost". By sending SIGHUP to the server process, one can now reopen the debug log file without losing any data.
|
Good suggestions. Updated. |
|
ACK, looks perfect to me. Thanks for your patience. |
Reopen log file on SIGHUP
8ee7b8a Move broadcast creation to CMasternodeBroadcast
This pull request is intended to facilitate bitcoind log rotation. When the daemon receives a HUP signal, it reopens the debug.log file so the previous one can be rotated. I've tried to describe the technical motivation in each of the three commit messages. It uses flockfile and funlockfile to avoid thread contention issues. As best I can tell, those functions are available on most platforms, but I have only compiled on OS X.
I have a test script which sends SIGHUP to the bitcoind process every 100 ms. I've been running with that test script and this patch for the last few days and haven't had any problems. In my tests, it behaves well during startup and shutdown too. Each rotated log contains the content I expect.
Suggestions for improvement welcome.